Size Change: -74 B (0%)

Total Size: 1.33 MB

_{compressed-size-action}

Flaky tests detected in 2cb814c.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4298422877
📝 Reported issues:

• [Flaky Test] default to the only existing classic menu if there are no block menus #48033 in /test/e2e/specs/editor/blocks/navigation.spec.js

Thanks for the PR, and thanks for the attention to detail 🙏

Looks much better in the case of the post date format toggle:

I'll refer to you and the components group on the details of the technical implementation, so the following is more of a question to satisfy my curiosity. Looking at the margins of the controls in the inspector, there are a few margins stacking and collapsing, like so:

Would it be possible to use gap to space things out instead of margin in cases like these? Or are there downsides there that I'm not aware of?

Would it be possible to use gap to space things out instead of margin in cases like these? Or are there downsides there that I'm not aware of?

Thanks for taking a look @jasmussen ! I would love to use gap here instead. But right now the inconsistent spacing is due to the Inspector's BaseControl margin override which many third-party blocks likely rely on.

The only way we could change it, in this case, is if we added another CSS override for the Post Date block to remove the BaseControl bottom margin. But I think adding more overrides will just become messy. As well, there are so many inconsistencies (😭) in the block inspector, that it's probably best to make the changes all at once (based on the plan here)? cc @mirka

CSS override currently on the date picker	Margin override from Inspector BaseControl

The CSS override is added to make the date picker toggle look consistent with the following two toggles, and so the CustomSelectControl component has spacing before the next toggle:

Thanks for taking a look @jasmussen ! I would love to use gap here instead. But right now the inconsistent spacing is due to the Inspector's BaseControl margin override which many third-party blocks likely rely on.

that it's probably best to make the changes all at once (based on #43423)?

Right, let's definitely not make overrides just for the post date use of the base control, definitely suggesting a refactor of the control overall.

The way I see it is that we are improving the WordPress component set for the long haul. In some cases that means revisiting past decisions, and making breaking changes. In the case of npm consumers, a clear readme before they npm update feels like a potential path forward. Not ideal of course, something to try and avoid when possible, but if we feel with high confidence that the path forward (in this case, gap instead of bottom margins) is the right one, then it is almost certainly going to be worth it from a pure maintenance and code quality perspective.

As far as spacing that third party blocks rely on, is there any way we can quantify how big of an issue it is? If we were to make a refactor across all usage of base control, would it be possible to identify problematic issues, and add a temporary hack to address those issues in the component itself? A hack with an end-date, something like that?

As far as spacing that third party blocks rely on, is there any way we can quantify how big of an issue it is? If we were to make a refactor across all usage of base control, would it be possible to identify problematic issues, and add a temporary hack to address those issues in the component itself? A hack with an end-date, something like that?

Those are all very good questions! I'm curious to hear what @mirka and @ciampo think about your questions/suggestions. 😄 To me, it sounds like it would be similar to the project this PR is a part of (#38730): adding an opt-in prop, migrating usages in Gutenberg, and then adding the deprecation warning. Or are there limitations on doing this for the inspector's bottom margin (aside from how time-consuming it would be 😅)?

adding an opt-in prop, migrating usages in Gutenberg, and then adding the deprecation warning. Or are there limitations on doing this for the inspector's bottom margin (aside from how time-consuming it would be 😅)?

Honestly I'm thinking even simpler: just making the change. No props, just better code and making the change in one sweeping way. Keep in mind I'm not a developer, and before doing this it would be good to boost the confidence on what effect this might have on 3rd party plugins. As in, try a couple of the most popular block editor plugins that use the base control, see what the patch might do. Mainly I have a feeling that it won't make a big impact, if any. Of course, I might be wrong! But even if there's a teensy bit of visual-only margin breakage in consumers that haven't used the base control correctly, it feels worth it for the long term gain.

Unfortunately we can't just rip off the bandaid because the impact will be substantial. Just looking at usages within core Gutenberg, the majority of inspector controls seem to be relying on the implicit margin-bottom added by the block-inspector stylesheet.

What we'll likely do is, once we implement the standard "controls grid" component (#43423), we'll counteract the block-inspector margin overrides inside the CSS scope of that grid. (Similar to how we're starting to do it with higher-level components like FocalPointPicker and QueryControl.)

So something like this in the block-inspector stylesheet:

.components-controls-grid .components-base-control {
  margin-bottom: 0;
}

This will help ensure a smooth transition by letting devs opt into the clean and consistent spacing, while also maintaining the legacy usages of those relying on the implicit margin-bottom.